Add 'unique-string' as an available type for marker payloads and schemas#4628
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #4628 +/- ##
=======================================
Coverage 88.47% 88.47%
=======================================
Files 295 295
Lines 26290 26299 +9
Branches 7097 7099 +2
=======================================
+ Hits 23260 23269 +9
Misses 2818 2818
Partials 212 212
☔ View full report in Codecov by Sentry. |
|
I'm letting Nazım look at this because he'll also look at the C++ side. |
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Thanks for working on this! That's a great start! I added some comments below, let me know what you think.
Also, I think it would be good to have some test coverage for this new field to make sure that we don't regress it in the future. Could you add one to src/test/unit/marker-schema.test.js ? (I think this is the best file) You'll probably need to generate a new string table or a thread and pass it around as well to test it. But it should be fairly straightforward by creating it with new UniqueStringArray(["string 1", "string 2", ...])
|
Also a nit: I think your git user might be misconfigured. It's using your mozilla email but it's not associated with this github account. You can either add your mozilla email to this github user from the settings or you can use your current user's email address to commit. Either way should work fine. |
|
@canova Thanks for the review, and the heads up about my git config! It should be easily sorted, and I'll make the requested changes ASAP. |
AdamBrouwersHarries
force-pushed
the
string-table-rendering
branch
from
490900f to
e38ab8d
Compare
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Looks great to me, thanks for updating the PR.
As we discussed on Wednesday, it would be good to add some comments. Also I think you decided to update the changelog in the end. Feel free to land this PR after you push the latest changes, and after you fix the merge conflicts.
AdamBrouwersHarries
force-pushed
the
string-table-rendering
branch
from
a08055a to
bdba850
Compare
|
Note: Due to some conflict issues I've done a local rebase, and had to force push (overwriting GitHub's own merge). |
3 participants
This PR adds a new format to the profile marker schema, namely
unique-string. This alows users to store indices into a string table as the data component of their marker, potentially saving space within a generated profile. This PR also bumps the gecko profile format version, as changes in the firefox backend which includeunique-stringdata will break profiler frontends without support.